implented time travel feature, will style later#2
implented time travel feature, will style later#2paulsobers23 wants to merge 5 commits intoThe-Marcy-Lab-School:masterfrom
Conversation
ipc103
left a comment
ipc103
left a comment
There was a problem hiding this comment.
Hey @paulsobers23 - really nice work on this one! The feature works really well, and using a context was a cool approach. I left some comments inline. Most of my comments are around figuring out what should be part of the context vs. what should live on the Board component directly. There's really no right or wrong answer, but it might be fun to think about what else could go onto the context. Please definitely let me know if you have any follow up questions!
| @@ -0,0 +1,8 @@ | |||
| import React from 'react'; | |||
|
|
|||
| function Cells(props) { | |||
There was a problem hiding this comment.
One small note - you can use de-structuring here if you like so you don't need to write props. in the markup.
There was a problem hiding this comment.
| function Cells(props) { | |
| function Cells({click, value}) { |
There was a problem hiding this comment.
One other tiny thing - I'd name this component Cell instead of Cells since it really just represents one cell. I'd expect a Cells component to render a whole list of cells.
|
|
||
|
|
||
| function Board(props) { | ||
| const { board, setBoard, turn, setTurn, history, setHistory } = useContext(BoardContext); |
There was a problem hiding this comment.
Using a context here is a cool choice! I think I'd be inclined to put even more logic into the context and let the board know even less. For example, I think your declareWinner and clickHandler, and travel functions could all go onto the context. It's possible that they could then replace the setBoard, setTurn, and setHistory functions. Basically, you can let the context do more of the heavy lifting. Let me know if this seems to make sense!
| <h3>{moves}</h3> | ||
|
|
||
| <div className="row"> | ||
| <Cells click={() => clickHandler(0) |
There was a problem hiding this comment.
I'd definitely recommend using map here for a couple of reasons. 1). It's a little bit more expressive, as it shows that you are rendering a cell for each item in the board and 2). It would be less for you to type :-)
{board.map((cellValue, i) => <Cells click={() => clickHandler(i)} value={cellvalue} /> }There was a problem hiding this comment.
Also, one advantage to moving the clickHandler function onto the context is that you wouldn't need to pass it as a prop- the Cell component could just pull the function it needed from the context on its own.
| status = `The winner is ${winner}`; | ||
| } | ||
|
|
||
| function clickHandler(num) { |
There was a problem hiding this comment.
I'd probably call this function move or makeTurn instead of clickHandler since that's a little bit more descriptive.
| setBoard(updatedBoard); | ||
| setTurn((turn === 'X') ? 'O' : 'X'); | ||
| const recordedHistory = history.slice(); | ||
| recordedHistory.push(updatedBoard); |
There was a problem hiding this comment.
Storing the entire board in the history definitely works - another approach is to store a list of turns and use that to render what the board looked like on a given turn. Something like [{turn: 1, value: 'X', cell: 5}, {turn: 2, value: 'O', cell: 3}]
|
|
||
| function travel(i) { | ||
| const boardInHistory = [...history]; | ||
| setBoard(boardInHistory[i]); |
There was a problem hiding this comment.
The game can get a little weird if you travel back to a previous move, then click on a square of the board. I might be inclined to use a isViewingHistory piece of state here to stop the player from selecting any squares unless they are at the current move.
3 participants
No description provided.